feat: add non-atomic multi-msg transactions #15038

nicolaslara · 2023-02-15T12:53:03Z

Description

This adds support for non-atomic multi-msg transactions.

It introduces a NonAtomic boolean field in the transaction proto that defaults to false. If this is not set, everything should behave as before. If set, the transaction will succeed if at least one message succeeds.

If all messages fail, the whole transaction will fail the same way as atomic transactions do.

State changes form failed messages should be reverted.

If the tx succeeds, but some messages failed. The resulting TxMsgData should contain an array of message responses that matches the order of the submitted messages. Successes will contain the MsgResponse and failures will contain an empty response created specifically for this: /cosmos.tx.v1beta1.MsgFailureResponse

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

tac0turtle · 2023-02-20T10:02:33Z

hey let us know when youd like some feedback on this pr

nicolaslara · 2023-03-08T13:15:23Z

@tac0turtle I don't need a full review rn, but some feedback on the general approach would be great! Basically, do we want to:

Add field to tx proto
Modify RunMsgs to do atomic vs non-atomic based on that field
Default to non-atomic

If so, then my next steps are:

Cache the context for each message
Check that results and errors are properly emitted and that we can map them to the original message

alexanderbez

The general idea of this makes sense to me. Is this something you intend to complete @nicolaslara? Really cool to see someone working on this 🙏

nicolaslara · 2023-03-08T15:14:35Z

yeah, I want to complete it soon, just didn't want to spend too much time on it before validating the approach.

nicolaslara · 2023-03-20T08:45:27Z

@tac0turtle @alexanderbez This is ready for feedback now.

There are two test failures that I'm not sure how to get around:

Concensus warn. It's unclear to me what this is checking.
Tx contains new fields (yes, that's on purpose)

Other than that I think this covers everything we need.

baseapp/baseapp.go

proto/cosmos/tx/v1beta1/tx.proto

types/errors/errors.go

amaury1093

Concept ACK.

proto/cosmos/tx/v1beta1/tx.proto

baseapp/baseapp.go

amaury1093 · 2023-03-22T12:20:25Z

x/auth/migrations/legacytx/stdtx.go

@@ -87,6 +87,7 @@ type StdTx struct {
 	Signatures    []StdSignature `json:"signatures" yaml:"signatures"`
 	Memo          string         `json:"memo" yaml:"memo"`
 	TimeoutHeight uint64         `json:"timeout_height" yaml:"timeout_height"`
+	NonAtomic     bool           `json:"non_atomic" yaml:"non_atomic"`


If we want to sign a NonAtomic tx with AMINO_JSON, we need to modify the ledger app to show this boolean field in a new screen.

I propose to never allow NonAtomic txs with SIGN_MODE_AMINO_JSON. Does this sound reasonable?

If so, could you add it in this PR, maybe in amino's sign mode handler?

That sounds reasonable. I'll look into it

Started adding the signing mode validation in osmosis-labs@09eb3a4. It's a bit messy because the tx has to be made into a concrete type, which normally doesn't happen in baseapp.

I'm not sure what a reliable way to test this would be though. Do you know of a good way to generate amino signed txs in tests that I can use for this?

amaury1093 · 2023-03-22T12:23:19Z

proto/cosmos/tx/v1beta1/tx.proto

+  //
+  // If set to true, the transaction will be executed as long as at least one
+  // of the messages succeeds.
+  bool non_atomic = 4;


Once this is merged, we need to add a field in the Textual enveloppe value renderer to show this field too. I added an item in #11970 to track this.

It's on this same PR now 👌

facundomedica

Looking good, thank you for adding the stuff needed for signmode textual. I'll test it today and approve 👌

facundomedica · 2023-04-17T13:05:24Z

x/consensus/keeper/keeper.go

+	if err := k.event.EventManager(ctx).EmitKV(
+		ctx,
+		"update_consensus_params",
+		event.Attribute{Key: "authority", Value: req.Authority},
+		event.Attribute{Key: "parameters", Value: consensusParams.String()}); err != nil {
+		return nil, err
+	}
+


what's the reason for this?

facundomedica · 2023-04-17T13:12:53Z

proto/cosmos/tx/v1beta1/tx.proto

+  //
+  // If set to true, the transaction will be executed as long as at least one
+  // of the messages succeeds.
+  bool non_atomic = 4;


It's on this same PR now 👌

facundomedica

LGTM. btw sign mode textual doesn't work with this as is but I'll have a fix soon, not caused by this pr tho.

colin-axner · 2023-05-16T15:39:21Z

baseapp/baseapp.go

 		if err != nil {
-			return nil, errorsmod.Wrapf(err, "failed to execute message; message index: %d", i)
+			wrappedErr := errorsmod.Wrapf(err, "failed to execute message; message index: %d", i)


Unfortunately, this is unsafe. The block header includes the following information as determinstic:

Code

Data (marshalled msg responses)

GasWanted

GasUsed

This combination creates a potential halting scenario. This code will include the full error string from the msg exeuction into the msg responses, thus causing the error string to know be included in the block. If the error string is undeterminstic (happens with json marshal errors) or it is changed (a backport changes error string formatting), a chain would halt. We have this same issue with writing error strings into acknowledgements for IBC, which is common occurrence with interchain accounts

To increase the readability of these error messages, I'd like to ask cometbft to include the codespace in the block header as well (I will open an issue), but I also think there should potentially be a more radical change to include the base error message as well. The original design was to use the Code as a condensed error string with the idea that that clients would lookup the code to get a more descriptive error string, but this is infeasible with IBC as a client would need to look up a different chain's error code

This makes sense. A simple solution would be to extract error code via errorsmod.ABCIInfo

the logic which is unsafe here seems to be the same as it was before, id opt to not return any sort of error to comet which gets included in consensus, (wiping the included data on errors that gets included in the header). I think this is fundamentally unsafe and its surprising no one has halted because of this.

The difference between before and after is that now the error is being included in the msgResponses which will eventually be marshalled and placed into the Data field (which is a determinsitic field).

Existing flow:

error returned on failed msg

which is passed back from runTx

which is put into a deliver tx response

and the non-deterministic error is removed

Thus the non-determinstic parts of an error returned on a failed msg execution are removed for the determinstic fields of the deliver tx response. In the new flow, the full error is put into the msg response below on line 843, which eventually ends up in the Data field which could lead to an app hash mismatch

One tricky part is that you likely want the errors to end up in the deliver tx response as well since the log is non-determinstic at the moment and can provide useful information

I think having the error that gets included in the error responses be just the ABCIInfo end emit the context as events or logs would solve this.

Also, hot take: consensus failures on error string differences should be irrelevant as long as all validators return an error, and there should be a way to fix this post-hoc at a performance penalty (though that's a much larger issue)

Made the change so that errors are converted to ABCI errors in 5a0ffba

github-actions · 2023-06-29T00:05:51Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tac0turtle · 2023-06-29T06:16:58Z

@testinginprod could we get a review on this pr

alexanderbez

few minor nits, otherwise ACK 🎉

baseapp/baseapp.go

08d2

🙅

Transactions are a concept that assert precisely one property: atomic execution. If a transaction doesn't provide atomic execution, then it is not a transaction, full stop.

You can define something else that provides partial execution, that's fine! But you can't redefine transaction to sometimes be atomic, and sometimes not. No go.

/cc @ebuchman @zmanian

odeke-em

Thank you @nicolaslara, please take a look at these few comments.

odeke-em · 2023-09-04T15:27:34Z

baseapp/abci_test.go

+	}
+
+	for testNum, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {


Please make a fesh copy of tc and testNum that ensure that the values will be afresh even if the test is parallelized, so:

testNum, tc := testNum, tc t.Run(tc.name, func(t *testing.T) { ...

odeke-em · 2023-09-04T15:28:19Z

baseapp/abci_test.go

+			// replace the second message with a Counter2
+			tx = newTxCounter(t, suite.txConfig, NonAtomic, int64(testNum+1), 3)
+
+			builder := suite.txConfig.NewTxBuilder()


Would be nice to declare this variable close too where it is used.

odeke-em · 2023-09-04T15:33:30Z

proto/cosmos/tx/v1beta1/tx.proto

+  // If set to true, the transaction will be executed as long as at least one
+  // of the messages succeeds.


Kind ping @nicolaslara!

odeke-em · 2023-09-04T15:36:44Z

proto/cosmos/tx/v1beta1/tx.proto

+  // If set to true, the transaction will be executed as long as at least one
+  // of the messages succeeds.


Kind ping @nicolaslara!

github-actions · 2023-10-05T00:04:50Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

initial setup with proto changes and tests

2ac2d26

github-actions bot added C:CLI C:x/auth labels Feb 15, 2023

nicolaslara changed the title ~~initial setup with proto changes and tests~~ Add non-atomic multi-msg transactions Feb 15, 2023

initial implementation

e1d5df4

nicolaslara added 3 commits March 8, 2023 12:50

Merge branch 'main' into nico/non-atomic-multi-msg-txs

b16a3ba

fixes

a9c9b72

fixed e2e tests

f468627

nicolaslara changed the title ~~Add non-atomic multi-msg transactions~~ feat: add non-atomic multi-msg transactions Mar 8, 2023

added missing method to mock

723140c

alexanderbez reviewed Mar 8, 2023

View reviewed changes

nicolaslara added 5 commits March 16, 2023 11:26

Merge branch 'main' into nico/non-atomic-multi-msg-txs

f01adc2

cache each message indivifually

4bdbbae

fixes after merge and lints

9b1f401

handling message responses as an array that includes failures

5751c39

gofumpt

cefd27a

nicolaslara marked this pull request as ready for review March 16, 2023 16:27

nicolaslara requested a review from a team as a code owner March 16, 2023 16:27

facundomedica reviewed Mar 20, 2023

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

tac0turtle reviewed Mar 21, 2023

View reviewed changes

proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved

tac0turtle reviewed Mar 21, 2023

View reviewed changes

types/errors/errors.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Mar 22, 2023

Implement SIGN_MODE_TEXTUAL #11970

Closed

39 tasks

amaury1093 reviewed Mar 22, 2023

View reviewed changes

nicolaslara added 3 commits March 27, 2023 13:49

small changes based on the review feedback

a4ce4ea

fixed test

8d4bc30

added validation to disallow legacy amino signing on non-atomic txs

09eb3a4

facundomedica reviewed Apr 17, 2023

View reviewed changes

facundomedica mentioned this pull request Apr 17, 2023

[textual]: Implement bool value renderer #15861

Closed

facundomedica approved these changes Apr 17, 2023

View reviewed changes

tac0turtle added 3 commits May 12, 2023 14:37

Merge branch 'main' into nico/non-atomic-multi-msg-txs

5eccd26

fix build

69e6204

fix build

3b8d73f

github-actions bot added the C:Rosetta Issues and PR related to Rosetta label May 12, 2023

colin-axner reviewed May 16, 2023

View reviewed changes

colin-axner mentioned this pull request May 16, 2023

Add codespace into the determinsticExecTxResult value cometbft/cometbft#844

Open

nicolaslara added 2 commits May 29, 2023 11:12

Merge branch 'main' into nico/non-atomic-multi-msg-txs

6657819

addapted to latest main

7e31859

github-actions bot added C:orm C:x/gov and removed C:x/consensus labels May 29, 2023

using ABCI errors

5a0ffba

github-actions bot added the Stale label Jun 29, 2023

github-actions bot removed the Stale label Jun 30, 2023

alexanderbez approved these changes Jul 17, 2023

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

baseapp/baseapp.go Outdated Show resolved Hide resolved

baseapp/baseapp.go Outdated Show resolved Hide resolved

tac0turtle added 5 commits August 14, 2023 11:31

Merge branch 'main' into nico/non-atomic-multi-msg-txs

81d56fc

apply bez suggestions

2b3ec86

apply bez suggestions

492b624

linting

0a7c8e9

fix lint

db882a5

08d2 suggested changes Aug 28, 2023

View reviewed changes

odeke-em reviewed Sep 4, 2023

View reviewed changes

github-actions bot added the Stale label Oct 5, 2023

github-actions bot closed this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add non-atomic multi-msg transactions #15038

feat: add non-atomic multi-msg transactions #15038

nicolaslara commented Feb 15, 2023 •

edited

Loading

tac0turtle commented Feb 20, 2023

nicolaslara commented Mar 8, 2023

alexanderbez left a comment

nicolaslara commented Mar 8, 2023

nicolaslara commented Mar 20, 2023

amaury1093 left a comment

amaury1093 Mar 22, 2023

nicolaslara Mar 22, 2023

nicolaslara Mar 27, 2023

amaury1093 Mar 22, 2023

facundomedica Apr 17, 2023

facundomedica left a comment

facundomedica Apr 17, 2023

facundomedica Apr 17, 2023

facundomedica left a comment

colin-axner May 16, 2023

nicolaslara May 16, 2023

tac0turtle May 16, 2023

colin-axner May 17, 2023

nicolaslara May 18, 2023 •

edited

Loading

nicolaslara May 29, 2023

github-actions bot commented Jun 29, 2023

tac0turtle commented Jun 29, 2023

alexanderbez left a comment

08d2 left a comment •

edited

Loading

odeke-em left a comment

odeke-em Sep 4, 2023

odeke-em Sep 4, 2023

odeke-em Sep 4, 2023

odeke-em Sep 4, 2023

github-actions bot commented Oct 5, 2023

		// If set to true, the transaction will be executed as long as at least one
		// of the messages succeeds.

feat: add non-atomic multi-msg transactions #15038

feat: add non-atomic multi-msg transactions #15038

Conversation

nicolaslara commented Feb 15, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

tac0turtle commented Feb 20, 2023

nicolaslara commented Mar 8, 2023

alexanderbez left a comment

Choose a reason for hiding this comment

nicolaslara commented Mar 8, 2023

nicolaslara commented Mar 20, 2023

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaslara May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2023

tac0turtle commented Jun 29, 2023

alexanderbez left a comment

Choose a reason for hiding this comment

08d2 left a comment • edited Loading

Choose a reason for hiding this comment

odeke-em left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2023

nicolaslara commented Feb 15, 2023 •

edited

Loading

nicolaslara May 18, 2023 •

edited

Loading

08d2 left a comment •

edited

Loading